Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rmkdir #2

Merged
merged 3 commits into from
Oct 29, 2023
Merged

rmkdir #2

merged 3 commits into from
Oct 29, 2023

Conversation

critBus
Copy link
Contributor

@critBus critBus commented Oct 21, 2023

Básicamente agregue un método nuevo a la interfaz com.nerox.client.callbacks.ITfprotocolCallback

default void rmkdirCallback(StatusInfo status){
        throw new RuntimeException("Callback is not implemented: exception");
    }

Y su correspondiente método a com.nerox.client.Tfprotocol

public void rmkdirCommand(String path) {
        this.getProtoHandler().rmkdirCallback(
                this.easyreum.getBuilder().build("RMKDIR",path).translate()
                        .getBuilder().buildStatusInfo());
    }

Add test com.nerox.client.RmkdirCommandTest which attempts to create a folder on the server to test the functionality of the new RMKDIR command added
Copy link
Contributor

@godjangollc godjangollc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The requested changes are for:
Keep a clean structure in the code
Delete changes not related to the PR (i.e .gitignore changes)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you create a whole new file for the test?? please use the already created file with unit tests and add the tests to that file, use AppTest.java, please notice that we use a different java class for each module, not each command... I think that's the last change we need to approve the commit. In the logic of the test please don't set in the same function that amount of steps because you can have errors for connecting, for invalid keys, for server errors, and a lot more errors not related to the command, as a result, we use a different approach, we do the connection in one function, and then we use the command in another one, for this particular command do client side validations and then assert to a certain condition to be true, then do server-side validation(i.e. stat command, etc.) .... Try to cover as most as possible in the units testings for each command... It will always be welcome more than one unit of testing per command... best regards

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is OK, please don't add any changes to it

.gitignore Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When publishing to a public repo, please refrain from uploading changes not related to the fix or feat that is being solved. This causes the reviewer (in this case me) to review more codes... I would appreciate if you delete this from the PR... thanks beforehand

The test is included within the com.nerox.client.AppTest class, represented by the testsRmkdir method
The previous test that was represented by the com.nerox.client.RmkdirCommandTest class is eliminated
The .gitignore file is restored
Copy link
Contributor

@godjangollc godjangollc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done and thanks you very much

@godjangollc godjangollc merged commit 758d1e5 into main Oct 29, 2023
1 check passed
@godjangollc godjangollc deleted the dev-rene branch November 1, 2023 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants